Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(prefer-importing-jest-globals): don't add imports in the middle of statements #1645

Merged
merged 3 commits into from
Sep 4, 2024
Merged

fix(prefer-importing-jest-globals): don't add imports in the middle of statements #1645

merged 3 commits into from
Sep 4, 2024

Conversation

ejzimmer
Copy link
Contributor

@ejzimmer ejzimmer commented Aug 29, 2024 •
edited
Loading

The current fixer implementation for prefer-importing-jest-globals inserts the import directly before the reporting node. This works fine when the reporting node is a function call.

describe(...)

results in

import { describe } from 'jest/globals';
describe(...)

But it breaks when the reporting node is part of a variable declaration

const onClick = jest.fn();

results in

const onClick = import { jest } from '@jest/globals';
jest.fn();

This change updates the fixer so new imports are inserted at the top of the file.

console.log('foo');
const onClick = jest.fn();

results in

import { jest } from '@jest/globals';
console.log('foo')
const onClick = jest.fn();

Existing imports are unchanged

console.log('foo');
import { describe } from '@jest/globals';
describe('the thing', () => {
  const onClick = jest.fn();
}

results in

console.log('foo');
import { describe, jest } from '@jest/globals';
describe('the thing', () => {
  const onClick = jest.fn();
}

Resolves #1634

@ejzimmer ejzimmer mentioned this pull request Aug 29, 2024
Closed
G-Rath
G-Rath requested changes Aug 30, 2024
View reviewed changes
Copy link
Collaborator

@G-Rath G-Rath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you undo all the unneeded indenting changes in the test cases please?

@ejzimmer ejzimmer force-pushed the improve-handling-of-import-placement branch from dbb9a35 to ecec8dc Compare August 30, 2024 03:18
@ejzimmer
Copy link
Contributor Author

could you undo all the unneeded indenting changes in the test cases please?

Oh yikes, yeah, clearly prettier did not have my back.

@ejzimmer ejzimmer force-pushed the improve-handling-of-import-placement branch from ecec8dc to 090fd91 Compare August 30, 2024 03:22
@ejzimmer ejzimmer requested a review from G-Rath August 30, 2024 03:55
G-Rath
G-Rath approved these changes Aug 30, 2024
View reviewed changes
Copy link
Collaborator

@G-Rath G-Rath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks fine, though I'd appreciate if you could cleanup the intending on the new cases (sorry, Prettier doesn't handle javascript-in-javascript blocks 😅) and explain a bit more about

Why didn't I just put the import at the top in both cases? Because it would have broken a couple of tests, eg:

I don't follow how it would have broken the test you've mentioned

@ejzimmer
Copy link
Contributor Author

ejzimmer commented Sep 2, 2024

Why didn't I just put the import at the top in both cases? Because it would have broken a couple of tests, eg:

I don't follow how it would have broken the test you've mentioned

That test places the require inside a function, and I just wasn't sure if that was important. Like, is requiring different modules inside of different functions something people do with commonjs modules? (I work almost exclusively with esmodules where obviously that just isn't a thing)

@G-Rath
Copy link
Collaborator

G-Rath commented Sep 2, 2024

What matters for us (mostly) is if its syntactically valid e.g. if we would end up duplicating a const in the same scope, that'd be bad; technically rules don't need to care about style because those that care should have lint rules that style anyway e.g. we don't need to bother figuring out if we should be using double or single quotes, or about placing semicolons.

We still do some of that stuff when it's easy because it can still be nice to do, but if a particular thing is very complex then we can choose not to take on that complexity.

In this case though I think putting it at the top of the file is the right move as it's likely we'll have multiple references throughout nested functions, though I think we should be fine to just always put the require at the very top of the file like you're doing for imports (but it's probably not the end of the world?)

@ejzimmer ejzimmer force-pushed the improve-handling-of-import-placement branch from 2f99c06 to f33e4d9 Compare September 2, 2024 22:46
@G-Rath G-Rath changed the title fix(prefer-importing-jest-globals): ensure imports aren't inserted in… fix(prefer-importing-jest-globals): don't add imports in the middle of statements Sep 4, 2024
@G-Rath G-Rath merged commit 9c4197c into jest-community:main Sep 4, 2024
44 checks passed
github-actions bot pushed a commit that referenced this pull request Sep 4, 2024
@semantic-release-bot
chore(release): 28.8.3 [skip ci]
497a500 
## [28.8.3](v28.8.2...v28.8.3) (2024-09-04)

### Bug Fixes

* **prefer-importing-jest-globals:** don't add imports in the middle of statements ([#1645](#1645)) ([9c4197c](9c4197c))
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 4, 2024

🎉 This issue has been resolved in version 28.8.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@MelvinBot MelvinBot mentioned this pull request Sep 26, 2024
Open
@qdraw qdraw mentioned this pull request Oct 4, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@G-Rath G-Rath G-Rath approved these changes

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[prefer-importing-jest-globals] autofix is broken
3 participants
@ejzimmer @G-Rath @ezimmer-atlassian